Repository navigation
Conversation
system_definition(s) takes the points and the springs the model integrates; topology_metadata(s) carries its structure document in a log under topology. KiteGeometry is pinned through [sources] until it is registered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1-Bort-1
left a comment
There was a problem hiding this comment.
Independent review (advisory)
Verdict: APPROVE WITH COMMENTS · 3 inline, 0 off the diff
Good
- Connectivity comes straight from
s.springs, so there is no second copy ofSPRINGS_INPUT, as the card says; checked againstinit_springs!/loop!insrc/KPS4.jl - KCU extra mass is
set.kcu_massand nots.masses[segments+1], so the tether half-masses thatloop!adds atKPS4.jl:484are not counted twice pos_ENUis anSVectorfield in KiteGeometry'sPoint, so the definition copies the pose and does not alias the liveMVectors; confirmed in_structure.jl:73unit_stiffness = axial_stiffness * lengthgives the stiffness per length that the schema field asks for (N); matches theset.axial_stiffness / L_0construction- Both model methods share one 3-argument assembler, so there is one path from model to definition
- Deferring
connectivity_shaandto_jsonto KiteGeometry and not adding a third preimage writer is the right call; the reason is in the card - The round-trip testset protects the actual contract, a saved log parsing back to the same structure document
Not good
src/system_definition.jl:17— All 9 kite springs getd_lineandrho_tether, but KPS4 puts no mass on them (the kite points already hold all ofset.mass), and several are canopy struts, not lines. Anything that sums segment mass will read a heavier system than the one that was simulated.src/system_definition.jl:52— A 7-argument positionalMetadata(...)with two empty strings, a version literal and another empty string can't be reviewed from the diff. Keyword construction, or a KiteGeometry default, would nameconnectivity_shaand make the placeholder visible.test/test-system-definition.jl:31— This compares an immutable snapshot with spring endpoints thatloop!never changes, so it passes whatever the step does. Thenext_step!and the assertion add runtime but protect nothing beyond a check before the step.- The
[sources]pin to an unregistered KiteGeometry means./bin/releasecannot register KiteModels until the pin comes out; merging to main blocks releases until then @reexport using KiteGeometryis only in the changelog; the card never gives a reason for pushingPoint,Segment,Tether,Winch,Metadatainto every user's namespace- The
"1.0.0"and"structure_schema.yml"literals copy constants KiteGeometry owns and will drift when its schema version moves stiffness_factoris left out ofunit_stiffness, so the example (stiffness_factor=0.1) logs 10x the stiffness it actually simulated; the docstring says nothing about this- The 3-argument
system_definition(s, segments, extra_masses)is a helper, but it becomes a documented method of an exported function - Prose in
docs/src/functions.mdand the changelog is hard-wrapped, against §6
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
|
Local full suite: PASS (2 min, Julia 1.13.0, one cell of the matrix) |
…nwrapped prose The bridle springs carry no mass in KPS4, so their segments get zero density. The shared constructor becomes assemble_system_definition instead of a third method of the exported system_definition. The KPS4 test drops a next_step! that no assertion depended on and pins unit_stiffness at the nominal value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Resolve examples/reel_out_4p_torque_control.jl: keep main's init! call without the deprecated delta and stiffness_factor, and keep this branch's topology metadata. Drop the deprecated delta from test-system-definition.jl's init! calls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
KiteGeometry.jl is released now. |
Drop the [sources] pin now that KiteGeometry 0.1.0 is in General. The log's topology is now the JSON that KiteGeometry.definition reads, and the definition's connectivity_sha is filled in, so YAML is no longer a direct dependency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The definition test reads the log back as JSON through KiteGeometry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
save_log should get a method override, automatically adding the metadata. I think this method override should be in KiteGeometry.jl. Also a convenience method to easily get the system definition from a loaded log. |
save_log(logger, system_definition(s), name) and KiteGeometry.definition(log) now live in KiteGeometry, so topology_metadata goes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Back in draft: its card waits on Depends-On: OpenSourceAWE/KiteGeometry.jl release after v0.1.0. |
|
Done. The save_log method and the log reader are in OpenSourceAWE/KiteGeometry.jl@b9583dd, whose pull request opens with this turn: save_log(logger, system, name) stores the definition, and KiteGeometry.definition(load_log(name)) reads it back. #327 switches to them in b44220a and drops its own helper. It is in draft until a KiteGeometry release carries the change. |
|
CI: https://github.com/OpenSourceAWE/KiteModels.jl/actions/runs/38356764958/job/115161368230 |
Main moved the default manifests to DSP 0.8.6. Both .defaults are main's plus a resolve that adds KiteGeometry; 1.13 was resolved under Julia 1.13.0, the newest on the box, so julia_version and LibSSH2_jll go back to 1.13.0's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Depends-On: OpenSourceAWE/KiteGeometry.jl release after v0.1.0
Until now a KPS3 or KPS4 run described its system only through a segment count and a few settings. Nothing that reads a system definition could draw it, KiteViewers included. Now the model can describe itself as a KiteGeometry system definition: its points, the springs it integrates as segments, one tether and one winch. That definition can be saved in the log, so a viewer reading the log gets the topology the run actually integrated. KiteGeometry is re-exported, as KiteModels already does for KitePodModels, WinchModels and AtmosphericModels.
What the definition holds:
"1"upwards. Point 1 is the winch and is static.init!. The stiffness is the nominal value. The model only scales it below nominal during a start-up ramp.topology. The definition carries its connectivity checksum, so a reader can tell when a log's states were recorded against a different topology.This needs OpenSourceAWE/KiteGeometry.jl#50, which adds the log methods. Until a KiteGeometry release carries it, this branch's tests fail in CI against KiteGeometry 0.1.0. Once it is released, the KiteGeometry compat entry here rises to that version, with a minimal manifest resolve.
Where I'd push back:
init!and passes it tosave_log. Its logger never sees the model, and KiteUtils' logger has nowhere to keep metadata, so an untouched script cannot carry the definition without global state. The simulation itself is unchanged.aero_force_bandtether_induced_forceevery step. It writes neither, so the KiteUtils state rename has nothing to change here.Dependencies: KiteGeometry 0.1.0, from the General registry. Both manifests come from a minimal resolve that started from main's. They gain the KiteGeometry entry and its JSON dependency. The 1.13 resolve ran under Julia 1.13.0, while main's 1.13 manifest was written under 1.13.1, so it also records
julia_version1.13.0 and the LibSSH2_jll that ships with 1.13.0.Verification
test/test-system-definition.jl: the log testset errored against KiteGeometry 0.1.0 (MethodError, since nosave_logtakes a definition there) and passes against the KiteGeometry branch. 16/16 across 3 testsets (juliaserver, Julia 1.13.0,testproject).examples/reel_out_4p_torque_control.jlruns to the end and logs 1800 rows of 11 points.KiteGeometry.definitionon the loaded log gives 11 points (length(kps4.pos)is 11) and 15 segments whose endpoints equal the model's springs (true). The tether is segments 1–6, the drum radius 0.1615, andunit_stiffnessmatches atstiffness_factor1.0.test/test-aqua.jl9/9 · docs build clean · up to date with main (b84950a, DSP 0.8.6 manifests).reuseon the box).init!.Opened elsewhere
Opened by
1-Bort-1, an AI agent working for @1-Bart-1.Closes #322 · task
KiteModels.jl-322